SPARK-7237 Clean function in several RDD methods by ted-yu · Pull Request #5959 · apache/spark

ted-yu · 2015-05-07T02:29:54Z

No description provided.

AmplabJenkins · 2015-05-07T02:32:09Z

Can one of the admins verify this patch?

rxin · 2015-05-07T05:48:25Z

core/src/main/scala/org/apache/spark/rdd/RDD.scala

my understanding is that mappartitionsWithIndex cleans, and thus anything in that is cleaned. maybe it's an incorrect assumption. cc @andrewor14?

Looks like Reynold is correct.
I can update the PR for collect() and undo the change for other methods touched.

I think collect might've been cleaned in DAGScheduler's runJob. Double check that.

I believe that @andrewor14 added some tests for closure cleaning as part of his recent ClosureCleaner patch; we might check whether that test suite covers these methods.

The test suite does not currently cover these methods because they are deprecated, but maybe we should just add them. (@JoshRosen is referring to ClosureCleanerSuite)

@rxin @ted-yu actually even though mapPartitionsWithIndex does clean already, it cleans the whole closure but not the ones used in the closure. In this case, I believe it's actually necessary to clean f here since we won't actually clean it from mapPartitionsWithIndex. For the same reason I believe we also need to clean constructA since it's a closure provided by the user.

andrewor14 · 2015-05-07T19:09:16Z

Hey @ted-yu would you mind adding [SPARK-7237] to the title? It's a convention we use for Spark PRs to track the issues.

andrewor14 · 2015-05-07T19:09:25Z

ok to test

AmplabJenkins · 2015-05-07T19:12:12Z

Merged build triggered.

AmplabJenkins · 2015-05-07T19:12:17Z

Merged build started.

SparkQA · 2015-05-07T19:12:50Z

Test build #32138 has started for PR 5959 at commit 6c124a9.

andrewor14 · 2015-05-07T19:13:03Z

@ted-yu Also it would be good to add test cases for the particular methods you are testing in ClosureCleanerSuite. There is already a test for this specific issue and it should be fairly straight forward to just add to it. Thanks.

SparkQA · 2015-05-07T20:56:02Z

Test build #32138 has finished for PR 5959 at commit 6c124a9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-05-07T20:56:07Z

Merged build finished. Test PASSed.

AmplabJenkins · 2015-05-07T20:56:07Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32138/
Test PASSed.

AmplabJenkins · 2015-05-07T21:12:12Z

Merged build triggered.

AmplabJenkins · 2015-05-07T21:12:18Z

Merged build started.

SparkQA · 2015-05-07T21:12:55Z

Test build #32150 has started for PR 5959 at commit 6846e40.

AmplabJenkins · 2015-05-07T21:17:13Z

Merged build triggered.

AmplabJenkins · 2015-05-07T21:17:18Z

Merged build started.

SparkQA · 2015-05-07T21:19:11Z

Test build #32151 has started for PR 5959 at commit 0c8d47e.

andrewor14 · 2015-05-07T21:22:47Z

core/src/test/scala/org/apache/spark/util/ClosureCleanerSuite.scala

note that the tests will fail because these closures are expected to have return statements in them. Can you model these closures after the other ones in this test?

andrewor14 · 2015-05-07T21:29:52Z

Hi @ted-yu I believe this also needs to cover mapPartitionsWithContext and filterWith

SparkQA · 2015-05-07T22:25:14Z

Test build #32151 has finished for PR 5959 at commit 0c8d47e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-05-07T22:25:19Z

Merged build finished. Test FAILed.

AmplabJenkins · 2015-05-07T22:25:19Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32151/
Test FAILed.

SparkQA · 2015-05-07T22:56:54Z

Test build #32150 has finished for PR 5959 at commit 6846e40.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-05-07T22:56:59Z

Merged build finished. Test PASSed.

SparkQA · 2015-05-08T16:27:37Z

Test build #32236 has finished for PR 5959 at commit f6014c0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-05-08T16:27:43Z

Merged build finished. Test PASSed.

AmplabJenkins · 2015-05-08T16:27:43Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32236/
Test PASSed.

ted-yu · 2015-05-08T18:13:01Z

@rxin @andrewor14
Please let me know if there is any review comment which I haven't addressed

andrewor14 · 2015-05-08T21:01:33Z

core/src/test/scala/org/apache/spark/util/ClosureCleanerSuite.scala

why is this commented out?

ted-yu · 2015-05-08T21:15:24Z

From https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32195/consoleFull :

[info] - filterWith *** FAILED *** (13 milliseconds)
[info] org.apache.spark.SparkException: Job aborted due to stage failure: Task 1 in stage 154.0 failed 1 times, most recent failure: Lost task 1.0 in stage 154.0 (TID 865, localhost): org.apache.spark.SparkException: RDD transformations and actions can only be invoked by the driver, not inside of other transformations; for example, rdd1.map(x => rdd2.values.count() * x) is invalid because the values transformation and count action cannot be performed inside of the rdd1.map transformation. For more information, see SPARK-5063.

AmplabJenkins · 2015-05-08T21:27:12Z

Merged build triggered.

AmplabJenkins · 2015-05-08T21:27:18Z

Merged build started.

SparkQA · 2015-05-08T21:29:05Z

Test build #32275 has started for PR 5959 at commit 56d7c92.

andrewor14 · 2015-05-08T21:36:47Z

core/src/main/scala/org/apache/spark/rdd/RDD.scala

@ted-yu I investigated this a little and this won't work because you're bringing in the whole SparkContext into the closure. Instead we need to clean the closures outside of mapPartitionsWithIndex as you have done in other methods. There is nothing special about filterWith.

once you have done that please uncomment the tests

ted-yu · 2015-05-08T22:50:04Z

Should have looked closer :-)

AmplabJenkins · 2015-05-08T22:52:11Z

Merged build triggered.

AmplabJenkins · 2015-05-08T22:52:19Z

Merged build started.

SparkQA · 2015-05-08T22:52:34Z

Test build #32279 has started for PR 5959 at commit f83d445.

SparkQA · 2015-05-08T23:09:35Z

Test build #32275 has finished for PR 5959 at commit 56d7c92.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-05-08T23:09:40Z

Merged build finished. Test PASSed.

AmplabJenkins · 2015-05-08T23:09:40Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32275/
Test PASSed.

andrewor14 · 2015-05-09T00:16:55Z

Great, merging this into master and 1.4. Thanks @ted-yu.

Author: tedyu <yuzhihong@gmail.com> Closes #5959 from ted-yu/master and squashes the following commits: f83d445 [tedyu] Move cleaning outside of mapPartitionsWithIndex 56d7c92 [tedyu] Consolidate import of Random f6014c0 [tedyu] Remove cleaning in RDD#filterWith 36feb6c [tedyu] Try to get correct syntax 55d01eb [tedyu] Try to get correct syntax c2786df [tedyu] Correct syntax d92bfcf [tedyu] Correct syntax in test 164d3e4 [tedyu] Correct variable name 8b50d93 [tedyu] Address Andrew's review comments 0c8d47e [tedyu] Add test for mapWith() 6846e40 [tedyu] Add test for flatMapWith() 6c124a9 [tedyu] Clean function in several RDD methods (cherry picked from commit 54e6fa0) Signed-off-by: Andrew Or <andrew@databricks.com>

SparkQA · 2015-05-09T00:36:01Z

Test build #32279 has finished for PR 5959 at commit f83d445.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-05-09T00:36:06Z

Merged build finished. Test PASSed.

AmplabJenkins · 2015-05-09T00:36:06Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32279/
Test PASSed.

Author: tedyu <yuzhihong@gmail.com> Closes apache#5959 from ted-yu/master and squashes the following commits: f83d445 [tedyu] Move cleaning outside of mapPartitionsWithIndex 56d7c92 [tedyu] Consolidate import of Random f6014c0 [tedyu] Remove cleaning in RDD#filterWith 36feb6c [tedyu] Try to get correct syntax 55d01eb [tedyu] Try to get correct syntax c2786df [tedyu] Correct syntax d92bfcf [tedyu] Correct syntax in test 164d3e4 [tedyu] Correct variable name 8b50d93 [tedyu] Address Andrew's review comments 0c8d47e [tedyu] Add test for mapWith() 6846e40 [tedyu] Add test for flatMapWith() 6c124a9 [tedyu] Clean function in several RDD methods

Clean function in several RDD methods

6c124a9

rxin reviewed May 7, 2015
View reviewed changes

Add test for flatMapWith()

6846e40

Add test for mapWith()

0c8d47e

ted-yu changed the title ~~Clean function in several RDD methods~~ SPARK-7237 Clean function in several RDD methods May 7, 2015

andrewor14 reviewed May 7, 2015
View reviewed changes

andrewor14 reviewed May 8, 2015
View reviewed changes

core/src/test/scala/org/apache/spark/util/ClosureCleanerSuite.scala Outdated

Copy link

Contributor

andrewor14 May 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this commented out?

Consolidate import of Random

56d7c92

andrewor14 reviewed May 8, 2015
View reviewed changes

Move cleaning outside of mapPartitionsWithIndex

f83d445

asfgit closed this in 54e6fa0 May 9, 2015

Conversation

ted-yu commented May 7, 2015

Uh oh!

AmplabJenkins commented May 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented May 7, 2015

Uh oh!

andrewor14 commented May 7, 2015

Uh oh!

AmplabJenkins commented May 7, 2015

Uh oh!

AmplabJenkins commented May 7, 2015

Uh oh!

SparkQA commented May 7, 2015

Uh oh!

andrewor14 commented May 7, 2015

Uh oh!

SparkQA commented May 7, 2015

Uh oh!

AmplabJenkins commented May 7, 2015

Uh oh!

AmplabJenkins commented May 7, 2015

Uh oh!

AmplabJenkins commented May 7, 2015

Uh oh!

AmplabJenkins commented May 7, 2015

Uh oh!

SparkQA commented May 7, 2015

Uh oh!

AmplabJenkins commented May 7, 2015

Uh oh!

AmplabJenkins commented May 7, 2015

Uh oh!

SparkQA commented May 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented May 7, 2015

Uh oh!

SparkQA commented May 7, 2015

Uh oh!

AmplabJenkins commented May 7, 2015

Uh oh!

AmplabJenkins commented May 7, 2015

Uh oh!

SparkQA commented May 7, 2015

Uh oh!

AmplabJenkins commented May 7, 2015

Uh oh!

SparkQA commented May 8, 2015

Uh oh!

AmplabJenkins commented May 8, 2015

Uh oh!

AmplabJenkins commented May 8, 2015

Uh oh!

ted-yu commented May 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ted-yu commented May 8, 2015

Uh oh!

AmplabJenkins commented May 8, 2015

Uh oh!

AmplabJenkins commented May 8, 2015

Uh oh!

SparkQA commented May 8, 2015

Uh oh!

Choose a reason for hiding this comment